Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Reflect for PhantomData #15313

Conversation

BenjaminBrienen
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen commented Sep 19, 2024

Objective

Solution

  • impl_reflect! following the example

Testing

N/A
I don't see tests for the other core impl_reflects.

@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Sep 19, 2024
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 19, 2024
@BenjaminBrienen
Copy link
Contributor Author

#5145 (comment)

I'm curious why we want this now and not before.

@MrGVSV
Copy link
Member

MrGVSV commented Sep 19, 2024

#5145 (comment)

I'm curious why we want this now and not before.

It's still the same issues as before. I think the biggest problem is serialization. It really only adds noise to the serialized output. And if we make it Reflect, then it becomes easy for library authors to forget to ignore it themselves, which means downstream users will see it if they serialize it.

If we do this, we may want to consider skipping it during serialization (and other operations such as reflect_partial_eq).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can we add a serialization test for PhantomData in the serde module? Whether or not we're allowing this type to be serialized, we should ensure any type containing it can be deserialized and FromReflect-ed.

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Sep 19, 2024
@soqb
Copy link
Contributor

soqb commented Sep 20, 2024

do we want really need this? i feel like PhantomData is supposed to act as if its not there at all (its a 👻 after all). is there anything not accomplished by annotating #[reflect(ignore)] or is it simply an ergonomic improvement? i'd really rather not have all the "noise" that @MrGVSV mentioned (not just in serialization mind, also in runtime and binary size) just for the sake of a few less tokens to type out.

tldr; the implementation is correct but i'm not sure the feature is desirable.

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR S-Needs-SME Decision or review from an SME is required and removed X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 20, 2024
@BenjaminBrienen
Copy link
Contributor Author

Maybe it would be better to instead implement a way to always ignore marker types?

@BenjaminBrienen
Copy link
Contributor Author

I don't know how to fix the unit test. Someone else can give me a pointer or do it themselves if they want. :)

@hymm
Copy link
Contributor

hymm commented Sep 22, 2024

Could there be a way of saying to ignore a whole type when you register it?

@alice-i-cecile
Copy link
Member

Yeah, that would be my preferred solution here.

@BenjaminBrienen BenjaminBrienen deleted the impl-Reflect-for-PhantomData branch September 22, 2024 19:53
@BenjaminBrienen BenjaminBrienen self-assigned this Nov 10, 2024
@BenjaminBrienen BenjaminBrienen added S-Wontfix This issue is the result of a deliberate design decision, and will not be fixed and removed S-Needs-SME Decision or review from an SME is required labels Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Wontfix This issue is the result of a deliberate design decision, and will not be fixed X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PhantomData does not implement Reflect
5 participants